Modernize CI workflow (GitHub Actions v4/v5, test matrix)#13
Modernize CI workflow (GitHub Actions v4/v5, test matrix)#13renecannao merged 4 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Modernizes the project’s CI setup by splitting the existing GitHub Actions workflow into dedicated lint, test (matrix), and build jobs, while also updating the Go toolchain baseline and related tooling/config.
Changes:
- Replaced the single CI job with separate lint/test/build jobs and upgraded Actions to checkout@v4 + setup-go@v5.
- Added a Go/OS test matrix (Go 1.22/1.23 × ubuntu/macos) and introduced golangci-lint + govulncheck in CI.
- Bumped
go.modto Go 1.22 and updated module dependencies (plus a few small Go code cleanups).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/all_tests.yml |
New multi-job workflow with lint/test matrix/build stages. |
.golangci.yml |
Adds golangci-lint configuration used by the new lint job. |
go.mod |
Raises Go version to 1.22 and updates direct/indirect dependencies. |
go.sum |
Syncs module checksums to the updated dependency set. |
.goreleaser.yaml |
Updates GoReleaser config (v2 schema + archive naming changes). |
ops/downloads.go |
Simplifies error formatting in tarball lookup path. |
ts/commands.go |
Fixes invalid regexp flags used in testscript helper. |
downloads/tarball_registry.go |
Removes Go build tags near embedded tarball registry. |
Comments suppressed due to low confidence (1)
downloads/tarball_registry.go:19
- Most files that use
//go:embedin this repo are guarded with//go:build go1.16(e.g.common/version.go,cookbook/cookbook_templates.go). This file now uses//go:embedwithout that build tag, which is inconsistent with the existing pattern. Either keep the tag here for consistency, or remove thego1.16build tags across the codebase if they’re no longer needed after raising the module to Go 1.22.
package downloads
import (
_ "embed"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return downloads.TarballDescription{}, fmt.Errorf(fmt.Sprintf("Error getting version %s (%s-%s)[minimal: %v - newest: %v - guess: %v]: %s", | ||
| version, flavor, OS, minimal, newest, guessLatest, err)) | ||
| return downloads.TarballDescription{}, fmt.Errorf("Error getting version %s (%s-%s)[minimal: %v - newest: %v - guess: %v]: %s", |
There was a problem hiding this comment.
This error should wrap the underlying err so callers can use errors.Is/As to inspect it. Use %w instead of %s for err (and consider starting the message with a lowercase letter to follow Go error conventions).
| return downloads.TarballDescription{}, fmt.Errorf("Error getting version %s (%s-%s)[minimal: %v - newest: %v - guess: %v]: %s", | |
| return downloads.TarballDescription{}, fmt.Errorf("error getting version %s (%s-%s)[minimal: %v - newest: %v - guess: %v]: %w", |
| amd64: x86_64 | ||
| - formats: ['tar.gz'] | ||
| name_template: >- | ||
| {{ .ProjectName }}_{{ .Version }}_{{ .Os }}_{{ .Arch }} |
There was a problem hiding this comment.
archives.name_template now produces assets like dbdeployer_<ver>_<os>_<arch>.tar.gz. This doesn't match the repository’s installer expectations (e.g. scripts/dbdeployer-install.sh downloads dbdeployer-<ver>.<os>.tar.gz). Either align the GoReleaser naming with the existing release/install contract, or update the installer/docs accordingly to avoid breaking installs from GitHub Releases.
| {{ .ProjectName }}_{{ .Version }}_{{ .Os }}_{{ .Arch }} | |
| {{ .ProjectName }}-{{ .Version }}.{{ .Os }} |
| run: | | ||
| go install golang.org/x/vuln/cmd/govulncheck@latest | ||
| govulncheck ./... | ||
| - name: Copyright check | ||
| run: ./scripts/sanity_check.sh copyright |
There was a problem hiding this comment.
The previous CI flow (scripts/ci.sh) ran ./scripts/sanity_check.sh (gofmt, go vet, version check, etc.). The new workflow only runs a copyright check + golangci-lint, and the current .golangci.yml does not enable a gofmt/gofumpt linter. If the goal is "no coverage lost" from the old CI, consider running ./scripts/sanity_check.sh (or at least restoring the gofmt/version checks) as part of the lint job.
| version: latest | ||
| - name: govulncheck | ||
| run: | | ||
| go install golang.org/x/vuln/cmd/govulncheck@latest |
There was a problem hiding this comment.
Using version: latest for golangci-lint makes CI non-reproducible and can cause sudden failures when a new linter release ships. Pin the golangci-lint version (and similarly consider pinning govulncheck) to a known-good version, then update it intentionally.
| version: latest | |
| - name: govulncheck | |
| run: | | |
| go install golang.org/x/vuln/cmd/govulncheck@latest | |
| version: v1.61.0 | |
| - name: govulncheck | |
| run: | | |
| go install golang.org/x/vuln/cmd/govulncheck@v1.1.1 |
| - ineffassign | ||
| - staticcheck | ||
| - unused | ||
| - gosec |
There was a problem hiding this comment.
The old CI sanity check explicitly enforced gofmt -l. With this linter set, formatting issues won’t be caught unless you add a formatter linter (e.g. gofmt/gofumpt/goimports) or run gofmt separately in CI.
| - gosec | |
| - gosec | |
| - gofmt |
| reLines := regexp.MustCompile(`(?sg)(^.*ERROR.*)`) | ||
| reLines := regexp.MustCompile(`(?s)(^.*ERROR.*)`) | ||
| errorLines := reLines.FindAll(contents, -1) | ||
| ts.Fatalf("ERRORs found in %s (%s)\n", logFile, errorLines) |
There was a problem hiding this comment.
In the neg && hasError branch, errorLines is a [][]byte, but the format string uses %s. This will print a formatting error (e.g. %!s([][]uint8=...)) instead of the matched lines. Consider converting the matches to strings and joining them, or use a verb that can print slices (e.g. %q/%v).
| ts.Fatalf("ERRORs found in %s (%s)\n", logFile, errorLines) | |
| var errorLinesStrs []string | |
| for _, line := range errorLines { | |
| errorLinesStrs = append(errorLinesStrs, string(line)) | |
| } | |
| ts.Fatalf("ERRORs found in %s (%s)\n", logFile, strings.Join(errorLinesStrs, " | ")) |
|
@gemini-code-assist review |
|
@claude review |
There was a problem hiding this comment.
Code Review
This pull request upgrades the project's Go version from 1.18 to 1.22 and updates numerous Go dependencies. It introduces a new .golangci.yml configuration to enable static analysis linters for improved code quality. The .goreleaser.yaml configuration is updated for enhanced release automation, including changes to archive formats and changelog filters. Additionally, an outdated Go build tag is removed from downloads/tarball_registry.go, an error message formatting is simplified in ops/downloads.go, and a redundant flag is removed from a regular expression in ts/commands.go.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
596fb16 to
e710427
Compare
Summary
lint,test, andbuildjobsCloses #3